test: #21 run the PostgreSQL integration tests in CI #72
Conversation
The database-backed suites were skipped in CI because nothing set TEST_DATABASE_URL, so "27 skipped" was passing for the whole database layer. The new `integration` job creates a throwaway PostgreSQL cluster inside the runner's temp directory, socket-only, applies the migrations, runs the server suite and deletes the cluster. No shared test database, no secret to hand out, and the application's `crate` database is never in reach. `deploy` now requires the job, so a deploy cannot ignore the database tests. The cluster alone would still have left the failure mode from "skipped" to "silently empty", so `server/scripts/assert-postgres-tests.mjs` reads the JSON report and fails when any database-backed file has zero executed or any pending assertions. Verified against both reports: with the database it prints the four files and exits 0, without it fails on the three suites that skip.
The first CI run failed with 'initdb: command not found' in 12 seconds: Debian puts the server binaries in /usr/lib/postgresql/<major>/bin and only the client on PATH, so PostgreSQL 18 being installed is not enough. Try PATH first, then the newest installed major version, and fail with a message that names the missing package if neither works.
There was a problem hiding this comment.
Nice work getting the PostgreSQL integration tests running in CI! The integration job is passing, but I found two things worth fixing before merging:
-
Skipped tests: The verification script doesn't catch individual tests marked as
skipped. This means some database tests could be skipped while the file still passes verification. Could we make sure the verifier catches unintentionally skipped database tests, while still allowing explicitly planned TODOs? -
PostgreSQL cleanup: The PostgreSQL binary path is only added to
PATHduring startup, so the cleanup step might not findpg_ctl. Also,PGDATAis only persisted after setup succeeds, which means cleanup might not work if initialization fails. Could we make both available to the cleanup step and ensure the server is stopped before removing its data directory?
One minor documentation suggestion: the README could clarify that the separate test database needs migrations applied before running tests locally.
Otherwise, the overall approach looks good!
Review by rachelks on #72 found that the guard only looked at whole files and at assertions with status `pending`. Vitest reports a skipped assertion as `skipped`, not `pending`, so that counter was always zero: a file with eleven passing tests and one `it.skip` was reported as fully executed. Demonstrated on a real report: flipping one assertion in musicBrainzCatalog.test.ts to `skipped` made the old guard print "4 passed" and exit 0. It now exits 1 with "3 passed, 1 skipped". `skipped`, `pending` and `disabled` all block. `todo` is allowed and counted in the output, so a test written down on purpose does not fail CI while an unplanned skip cannot hide. The check is now `checkReport()`, called by the CLI, so it can be covered without spawning a process. Eight tests: the passing case, a missing file, a file where nothing ran, one skipped assertion in a passing file, a failed file, an allowed todo, and the CLI exit codes for both outcomes. The suite list is imported from the script, so adding a database suite does not silently weaken the test.
Review by rachelks on #72: the cleanup step could not find what it needed. The PostgreSQL binary directory was added to PATH by `export` inside the start step, which does not reach a later step, and PGDATA was written to GITHUB_ENV at the end of that step, so neither existed if initdb or createdb aborted the job. Cleanup then ran with an empty PGDATA and a missing pg_ctl. Both are now resolved in a "Locate PostgreSQL" step that runs before anything can fail: the bin directory goes to GITHUB_PATH, PGDATA to GITHUB_ENV. Cleanup therefore has both whether or not a cluster was ever created. Cleanup also refuses to delete the data directory while the server is still running. `pg_ctl stop || true` used to hide a failed stop and the following `rm -rf` then removed the directory under a live postmaster. It now checks that pg_ctl exists, waits for postmaster.pid to disappear and fails instead of deleting. Verified all three paths by running the step body: PGDATA unset exits 0, a directory with a postmaster.pid and no usable pg_ctl exits 1 and keeps the directory, and a real throwaway cluster stops and is removed.
Review by rachelks on #72: the local instructions pointed TEST_DATABASE_URL at a database but never said the schema has to exist. The suites query tables, they do not create them, so a fresh database fails with "relation does not exist". The snippet now applies the migrations to the same URL first. docs/VM-setup.md also notes that the cleanup step runs after a failure and will not delete a data directory under a running server.
|
Thanks — both were real. Fixed in 1. Skipped tests were invisible You were right, and the cause is worse than a missing check: the script counted assertions with status Reproduced against a real report: I flipped one assertion in
The check moved into 2. Cleanup could not find PGDATA or pg_ctl Also correct: There is now a Cleanup also no longer deletes under a live server. 3. Migrations before tests The snippet now applies migrations to the same URL first, and says the suites expect the schema rather than create it: export TEST_DATABASE_URL=postgres://t19:password@127.0.0.1:5432/crate_test
DATABASE_URL="$TEST_DATABASE_URL" pnpm --filter @t19/server db:migrate
pnpm --filter @t19/server test
Re-review when you have a moment — this one unblocks #75, which |
Closes #21 (the remaining acceptance criterion: the tests run in CI without a manual setup step).
What was done
The database-backed suites (
facets,search,sortand the MusicBrainz import) were already written and already correct, but every one of them is wrapped indescribe.skipIf(!process.env.TEST_DATABASE_URL). CI never set that variable, sopnpm testreported 61 passed | 27 skipped and stayed green with the entire database layer missing. That is the one part of #21's checklist that was open.integrationjob in.github/workflows/ci.yml. It creates a PostgreSQL cluster withinitdbin$RUNNER_TEMP, listens on a unix socket only (listen_addresses=''), applies the migrations, runsvitestwith both the default and JSON reporters, and stops and deletes the cluster in anif: always()step.TEST_DATABASE_URLsecret: there is nothing to provision by hand and nothing to drift. The application database is not reachable from the job, sodb:seed/db:seed:largecannot be pointed at it by accident.deploynow hasneeds: [quality, integration], so a commit cannot deploy with the database tests untested (same shape as Deploy the server build to the path crate-api expects #44, one layer down).server/scripts/assert-postgres-tests.mjs: reads the JSON report and fails when a database-backed file executed zero assertions or left any pending. Without it the failure mode only moves from "suite skipped" to "suite silently empty". The file list is explicit so adding a new database-backed suite is a deliberate edit.docs/VM-setup.mddescribe how CI runs them and how to run them locally.What was tested
The whole job was rehearsed locally against the same commands, in a fresh temporary cluster, from
initdbto deleting it:The guard was checked against a deliberately broken run as well. Same tree,
TEST_DATABASE_URLunset:pnpm format:check && pnpm lint && pnpm typecheck && pnpm testpasses locally.Known risk
initdb/pg_ctlmust be on the runner'sPATH. PostgreSQL 18 is installed on the VM, so this should be/usr/lib/postgresql/18/bin; if the runner user'sPATHdoes not include it the job fails loudly on the first step and it is a one-line fix. Worth watching the first run of this PR.